Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement logarithmic quantities #103

Merged
merged 12 commits into from
Oct 23, 2017
Merged

Implement logarithmic quantities #103

merged 12 commits into from
Oct 23, 2017

Conversation

ajkeller34
Copy link
Collaborator

Here's my first pass at implementing logarithmic quantities. It seems to mostly work, but I would appreciate hearing about things that don't work but should, things that do work but shouldn't, and any objections to design decisions. In a perfect world I would keep this open for a week or so, then merge and tag, but that will depend on how much feedback I get.

Please see docs/src/logarithm.md for a full description.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 36aa3a5 on logunits into ** on master**.

@mweastwood
Copy link
Contributor

Wow there's an impressive amount of work that's gone into this. It looks really nice!

However, I find this decision really confusing:

julia> 10dBm + 10dB
20.0 dBm

julia> 10dBm + 10dBm
13.010299956639813 dBm

I think there is an argument to be made that 10dBm * 10dB == 20dBm. I believe this makes the most sense for generic programming.

For example, my apply_gain function below does the right thing in every case except when the power is given in units of dBm and the gain is given in units of dB.

julia> apply_gain(input_power, gain) = input_power * gain
apply_gain (generic function with 1 method)

julia> apply_gain(10mW, 100)
1000 mW

julia> apply_gain(uconvert(dBm, 10mW), 100)
30.0 dBm

julia> apply_gain(10mW, 20dB)
1000.0 mW

julia> apply_gain(uconvert(dBm, 10mW), 20dB)
ERROR: logarithmic gains add, not multiply.
Stacktrace:
 [1] apply_gain(::Unitful.Level{Unitful.LogInfo{:Decibel,10,10},1 mW,Quantity{Int64, Dimensions:{𝐋^2 𝐌 𝐓^-3}, Units:{mW}}}, ::Unitful.Gain{Unitful.LogInfo{:Decibel,10,10},Int64}) at ./REPL[34]:1

For similar reasons, I also think uconvert(dB, 100) should give 20 dB.

Additional food for thought. In astronomy, the brightness of a star is measured in terms of "magnitudes", which are computed as -2.5*log10(intensity / reference intensity). The negative sign is what's interesting here because it means the magnitude scale runs backwards. When it becomes possible for users to define their own logarithmic units, I'd like for that negative sign to not be a deal breaker. Is that going to be possible?

@ajkeller34
Copy link
Collaborator Author

Some responses:

  • Consider that 10dBm == 10mW. Since the reference level is specified explicitly when using dBm, there is a one-to-one correspondence between some power in dBm and some power in mW. So I'd better have that 10mW+10mW == 10dBm+10dBm. Doubling the power should give roughly a 3dB increase in level, not a 10dB increase in level. I'm taking the position that adding the numbers out front linearly isn't the correct thing to do here, but rather adding the physical powers is what is important. The situation is different for dB because no one-to-one correspondence can be drawn, people usually use dB to indicate gain or attenuation, and there's the notion of gains adding on a log scale.

  • I don't see a problem with defining 10dBm*10dB == 20dBm, however. I think there just needs to be a consistent choice for a "gain chaining operator." Usually we think of gains multiplying, but in logarithmic scales they should add. However, for the sake of generic programming, it may be that we want to just call that operation multiplication. It's pretty easy to write something that sounds good and then find yourself stuck in some not-totally-consistent corner...

  • uconvert(dB, 100) could equal 20dB or 40dB depending on if 100 is interpreted as a ratio of power quantities or root-power quantities (e.g. 100V/1V). One could simply argue that historically dB was defined for power quantities but people certainly use it in either case, so I'm not sure that is a good argument. This is why I implemented powerratio and rootpowerratio (or fieldratio).

  • The negative sign will not be a deal breaker, any prefactor / logarithm base is possible. I haven't implemented the powers of ten logic so right now you can only use decibels, not bels, etc.

@mweastwood
Copy link
Contributor

I am in support of 10dBm + 10dBm == 13.010299956639813 dBm. It's 10dBm + 10dB == 20.0 dBm that feels inconsistent to me.

I think the current design has two contradicting philosophies.

The first is that a linear and logarithmic units are interchangeable. This means that if I write a function that operates on a power quantity, I shouldn't have to care if that power is given to me in mW or dBm.

The second is that logarithmic units should operate in the way you expect. If I have a two 10dB amplifiers, intuitively I expect that I should be able to add the two values together to get the total gain. 10dB + 10dB is a sensible operation to write down and 10dB * 10dB is nonsense.

The first implies that the quantity x*dBm should operate as if I wrote down 10log10(x)*mW. By principle of least surprise (and ignoring root-power quantities for the moment), I would therefore also expected x*dB to operate as if I wrote down 10log10(x). But if that's the case x*dBm + y*dB can't make sense because one term has units of power and the other term is unitless.

Either we have to live with a surprising behavior that makes it difficult to write generic functions that accept both logarithmic and linear units, or we have to accept that gains are applied by multiplication in all cases (and forbid 10dB+10dB == 20dB).

As for the root-power quantities vs power quantities, maybe it makes sense to force the user to state which one they want when they write down dB? As in, define something like dB_power and dB_root_power to handle both cases and set dB to be handle whichever case you think is more common?

@ajkeller34
Copy link
Collaborator Author

Fortunately it's not a large design change to make 10dBm*10dB == 20dBm work and forbid 10dBm+10dB, and I'm fine with making that change for the sake of generic ("practical") programming. This stood out in my head as well as a potential problem when I was coding all this up.

It is worth keeping in mind that 10dBm + 10dB is distinct from 10dB+10dB as far as the types are concerned. We don't necessarily have to forbid 10dB+10dB just because we forbid 10dBm+10dB. If we forbid 10dB+10dB == 20dB, do we also forbid 2*10dB == 20dB? Because then we could not do e.g. 2dB/m * 3m == 6dB, at least as currently implemented, and that's a common way to think about signal attenuation in microwave cables, where the attenuation is actually exponential in length.

Regarding your last point, neither is more common, or at least whatever decision I make would be disappointing to a fair number of people. You can measure scattering parameters with a network analyzer and get magnitudes in dB corresponding to voltage ratios, which I do pretty regularly. Of course signal power attenuation (power ratios) are also pretty important. I think it is important to avoid silently returning the wrong answer when one expects something to work in their context, since that is also surprising behavior.

… multiplication tables in docs based on latest behavior.
@PainterQubits PainterQubits deleted a comment from codecov-io Sep 29, 2017
@PainterQubits PainterQubits deleted a comment from coveralls Sep 29, 2017
src/logarithm.jl Outdated
irp = isrootpower(x)
str = ifelse(irp, "root-power", "power")
warn("result may be incorrect. Define ",
"`Unitful.isrootpower(::Type{<:Unitful.LogInfo}, ::typeof($y))` to fix.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little wary of encouraging the user to change the behavior of Unitful because it makes the package behavior unpredictable. If I write some code using Unitful and copy it over to somebody else's system, I wouldn't want it to do a different thing just because they defined isrootpower differently in their .juliarc.jl for some units.

Maybe keep the behavior of dB as is, but in ambiguous cases where it's not clear whether you have a power or root-power quantity, then you can warn that the results may be incorrect, but encourage people to use either dB_power or dB_amplitude for which isrootpower is always false and true respectively.

That way if I'm operating on voltages or powers, I can do x*10dB because the convention is clear, but if I'm operating on some other quantity then I have to specify x*20dB_power -> 100x or x*20dB_amplitude -> 10x.

Just a thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see where you're coming from but I'm not sure the danger is high in this case. I guess you need to appeal to some physical laws to decide whether to use root-power or power for a quantity of given dimension. I can't think of any examples where the decision would be ambiguous. In other words, the behavior will not depend on user preferences: I would expect all users should define isrootpower the same way for a given dimension, and if they don't, they would be responsible for the error (or receive a warning if they don't define anything at all).

Anyway, rather than defining dB_power or dB_amplitude, I guess I'd prefer an alternative, such preferring to return no results rather than possibly erroneous ones, and encouraging a pull request rather than local modifications. It'd be nice to "crowd source" the correct answers for each dimension in the absence of some general algorithm.

@codecov-io
Copy link

codecov-io commented Sep 29, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@dac5a44). Click here to learn what that means.
The diff coverage is 83.85%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #103   +/-   ##
========================================
  Coverage          ?   84.3%           
========================================
  Files             ?      15           
  Lines             ?     803           
  Branches          ?       0           
========================================
  Hits              ?     677           
  Misses            ?     126           
  Partials          ?       0
Impacted Files Coverage Δ
src/Unitful.jl 100% <ø> (ø)
src/conversion.jl 90.24% <100%> (ø)
src/promotion.jl 54.83% <100%> (ø)
src/temperature.jl 100% <100%> (ø)
src/range.jl 85.18% <33.33%> (ø)
src/pkgdefaults.jl 26.08% <42.85%> (ø)
src/logarithm.jl 65.46% <65.46%> (ø)
src/user.jl 89.9% <80%> (ø)
src/types.jl 93.33% <89.47%> (ø)
src/units.jl 90.9% <90.9%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dac5a44...bc142d2. Read the comment docs.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c31e249 on logunits into ** on master**.

@mweastwood
Copy link
Contributor

I briefly experimented with the new user log units.

Unitful.@logscale mag    "mag"      Magnitude    10    -2.5    false
Unitful.@logunit  mag_AB "magᴬᴮ"    Magnitude    3631Jy

(where Jy is defined here)

However, I immediately ran into

julia> 1*UnitfulAstro.mag_AB
ERROR: undefined behavior. Please file an issue with the code needed to reproduce.
Stacktrace:
 [1] *(::Int64, ::Unitful.MixedUnits{Unitful.Level{Unitful.LogInfo{:Magnitude,10,-2.5},3631 Jy,T} where T<:Number,Unitful.FreeUnits{(),Unitful.Dimensions{()}}}) at /home/michael/.julia/v0.6/Unitful/src/logarithm.jl:94

Does this mean isrootpower needs to be defined for some new dimension? Something like

isrootpower_dim(::Type{<:LogInfo}, ::typeof(dimension(Jy))) = false

@ajkeller34
Copy link
Collaborator Author

Yes, that's right, I'll add something like that in the next commit.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f4c73d6 on logunits into ** on master**.

@ajkeller34
Copy link
Collaborator Author

Ok, I'm somewhat happy with the current state of things. I added some functionality to allow for dimensionless reference levels, which enables dBFS (reference level is 1). Also, 3dBm isa Unitful.Power now. I'm not sure the implementation of mixed log/linear units will ever feel clean to me, but that's because it's kind of ugly by nature...

The remaining obviously wonky thing is that we have both 20dB+20dB == 40dB and 20dB*20dB == 40dB, the former being the way the operation is typically expressed and the latter being useful for generic programming on gain factors. It's not obvious that the current situation poses a problem, though... what else could those operations mean?

I updated the docs for this branch again, should be easier to read than looking through the markdown files: http://ajkeller34.github.io/Unitful.jl/logunits/

Any other concerns? Maybe we should keep this open a little longer while you see if you encounter issues when playing with magnitude scales?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 64e1f03 on logunits into ** on master**.

@mweastwood
Copy link
Contributor

what else could those operations mean?

Apparently this:

20dB + 20dB = 23.01dB
www.wolframalpha.com/input/?i=20dB+%2B+20dB

20dB * 20dB = 400dB^2 wut
www.wolframalpha.com/input/?i=20dB+*+20dB

Maybe we should keep this open a little longer while you see if you encounter issues when playing with magnitude scales?

I'll give it a shot when I find some more free time. You could also just merge and stick a big experimental warning on it. That'll get more people experimenting with log units, which will hopefully make it clear where the pain points are in the current design.

@ajkeller34 ajkeller34 merged commit 3a86b33 into master Oct 23, 2017
@ajkeller34 ajkeller34 deleted the logunits branch October 23, 2017 16:39
@m-wells
Copy link

m-wells commented Mar 27, 2019

I've been struggling with this all day and finally got it to work. I had to do a lot of digging and I think the documentation could benefit from some clarification. I deal with logarithmic quantities all the time in astronomy so this is quite useful to me.

julia> using Unitful

julia> import Unitful:d

julia> @logscale Log₁₀ "Log₁₀" LogBase10 10 1 false
Log₁₀

julia> @logunit log₁₀d "log₁₀(days)" LogBase10 1d
log₁₀(days)

julia> Unitful.isrootpower_dim(::typeof(dimension(d))) = false

julia> foo = 3*log₁₀d
3.0 log₁₀(days)

julia> uconvert(u"d", foo)
1000.0 d

julia> bar = 0.3*log₁₀d
0.3 log₁₀(days)

julia> uconvert(u"d", bar)
1.9952623149688795 d

The isrootpower_dim is required to avoid the following:

julia> foo = 3*log₁₀d
ERROR: undefined behavior. Please file an issue with the code needed to reproduce.
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] isrootpower_dim(::Unitful.Dimensions{(Unitful.Dimension{:Time}(1//1),)}) at /home/mark/.julia/packages/Unitful/W0mMi/src/logarithm.jl:200
 [3] isrootpower(::Quantity{Int64,𝐓,Unitful.FreeUnits{(d,),𝐓,nothing}}) at /home/mark/.julia/packages/Unitful/W0mMi/src/logarithm.jl:199
 [4] fromlog(::Type, ::Quantity{Int64,𝐓,Unitful.FreeUnits{(d,),𝐓,nothing}}, ::Int64) at /home/mark/.julia/packages/Unitful/W0mMi/src/logarithm.jl:116
 [5] *(::Int64, ::Unitful.MixedUnits{Level{Unitful.LogInfo{:LogBase10,10,1},1 d,T} where T<:Number,Unitful.FreeUnits{(),NoDims,nothing}}) at /home/mark/.julia/packages/Unitful/W0mMi/src/logarithm.jl:147
 [6] top-level scope at none:0

@giordano
Copy link
Collaborator

I deal with logarithmic quantities all the time in astronomy so this is quite useful to me.

Side comment just to say that you may be interested in https://github.com/JuliaAstro/UnitfulAstro.jl, if you didn't already see it

@m-wells
Copy link

m-wells commented Mar 27, 2019

@giordano I do indeed use UnitfulAstro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants